-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Delaying mode-hooks when opening Org files for searching #200
base: master
Are you sure you want to change the base?
Conversation
With this change, and customizing org-ql-open-buffers to it's non-default value will give HUGE performance improvements to org-ql queries when files are queried that aren't already loaded into buffers. This change does not modify any defaults and org-ql will work just as before (and as slow) until org-ql-open-buffers is modified by the user. This is due to backwards compatability. * org-ql.el (org-ql-select): Don't always read files into buffers, depending on user customization. (org-ql--prepair-work-buffer, org-ql--work-buffer) (org-ql--init-work-buffer): Helper functions to work with new hidden work-buffer. (org-ql--work-buffer-name): New variable for name of work-buffer. (org-ql-open-buffers): User customization to choose whether org-ql should open all files in separate buffers or not.
prepair -> prepare
You are Gustav Wikstrom, I presume. :) This is an interesting idea. Is the performance improvement primarily from using A problem I guess a "work buffer" would have is that, when the results are presented in an Regardless of that, the "work buffer" approach could be useful in, e.g. Another potential issue with "work buffers" is that some of the Org mode hooks may affect some search results, e.g. setting file-level tags, so delaying those hooks might cause searches to return incorrect results. It might be tricky to account for that. What do you think? Thanks. |
Yes. Haven't profiled the separate parts but that is definitely where the major speedup comes from.
Indeed, one piece of code I have currently tries to look up the filename for each item in the query and that obviously doesn't work with this customization active. Similar incompatability for
Yeah, agreed. The dynamics of Emacs is a bugger and a blessing at the same time. Not sure how to act here really. Maybe still allow for this customization since it's not on by default, and provide a better explanation in the docstring of the risks? It really is a do or die change when the number of files grow large. In that context Org ql is VERY useful with this change, and not so much useful without. And the cases when correctness and mutations (or file lookup) are needed can be catered to case by case (eg. by using I'm sure more clever people than me can make this integrate better with the rest of Org ql. But wanted to get this out there anyhow due to the huge performance improvement. Extra remark: oh, also couldn't understand why the tests failed on snapshot. But I'm running snapshot Emacs myself now and this change works locally at least. |
Hello, thanks for this PR. I'm interested in it, because when it is merged, I will have to change the code of one of my packages.
I've used a similar technique in my org-multi-wiki package, which I think is used exclusively by myself at present. I learned this technique from #88 (comment) posted by @Kungsgeten. And indeed, the activation issue has existed. Here is a list of workarounds I am using:
While the technique definitely improves performance, I'm not sure whether it should be implemented in
I was unaware of this issue, and the global minor mode may not handle this case properly. |
Is it because of font-lock by any chance? If so, the problem may be fixed in org-mode itself in reasonable future. I have a working patch in https://github.com/yantar92/org. |
Yeah, definitely credit to @Kungsgeten for all of my code as well. Org-brain was first out with this optimization! 👌👍 |
I doubt that font-lock is the root to the problem here. Kind of hoping to be proven wrong though! Looking forward to seeing your font-lock code in Org none the less. As any improvement to Orgs performance is a huge win! |
Font-locking in org means that all the links have to be parsed for example. That involves a lot of regex search. In my org files, font-locking took nearly half of the startup time (also thanks to pretty-symbols). In any case, I would be curious to know about startup bottlenecks in your scenario.
Any feedback would be appreciated. That patch is complex and font-locking staff is just one of the optimisations. |
I did a benchmark from your branch @yantar92. No performance improvement. Slower actually. Output of the tests: (Run on different hardware than above, hence the difference in time from first post. This test: high-end AMD desktop. Tests in first post: virtual machine on high-end Intel laptop.)
First row is with new customization on. Second with it off (showing current performance) and third is with all files already loaded into buffers. I can note that benchmarking (benchmark 1 '(org-roam--list-all-files)) results:
These numbers are with my personal setup though, Emacs 28.0.50 on Windows 10 with native compilation on. Surely they can differ in another persons config but I suspect the relative performance difference is around the same. (i.e. around 100x faster with this customization) Sidenote: Got an error with the |
Clearly, font-lock is not the coolprit for you. Can you try to test a little differently to get more information:
|
cpu profiling doesn't give many clues, sorry to say. The following code is run two times, with only (setq gw/files (org-roam--list-all-files)) ;only run in first test
(let ((org-ql-open-buffers t))
(benchmark 1 '(org-ql-query
:select #'(org-get-heading t t t t)
:from gw/files
:where '(and (property "ID"))))) And the results:
|
The profiler report looks very suspicious.
Did it really take over one minute to run this? Looking at the org-roam's source code, this functions just finds all org files in directory. It should not take that much time.
20 seconds to load modules? Also very suspicious. I suspect that you somehow executed |
Yes it does take that time. Probably not CPU-bound. It can be an issue related more to Windows than Linux though. Not sure how multi-file Org ql queries behave on Linux. But on Windows 10 (on a pretty beefed up hardware spec as well!) the numbers reported above is what I see. No fabrication. Would be good to have a public set of files to run the same test on, to make it reproducible.. If you don't have access to a Windows config then you have to trust me for now.
I think you're mixing up CPU time with real time. The profiler doesn't seem to capture both.
I've started each test with As I've understood from previous use of |
I don't mind "hosting" discussion relevant to improving performance of Org-related code on my repo, but it does make it difficult to know where the discussion specific to this PR stands and what should be done next for it. @Whil- Would you please write some kind of status update about this PR's code and how it relates to org-ql specifically? For benchmarking, I generally recommend using these macros I wrote: https://github.com/alphapapa/emacs-package-dev-handbook#bench-multi-lexical They present results in a helpful way, and they help ensure that the code is doing the same thing each time it's run (several times it's caught mistakes of mine that would have invalidated the results). They also ensure that lexical-binding is used, which can affect the results. Also, while I'm very enthusiastic about the native-comp branch, of course, I'm not sure if it's a good idea to use it when benchmarking packages, especially if different platforms or users are involved. BTW, please see this old feature branch in @akirak I don't aim to break any APIs that would require changes in your packages. If some of the ideas in this PR make it into org-ql, they should probably not be used by default, and they should probably be activated by let-binding a variable around relevant code, and the developer who does so should know what he's doing (so we would need to document the feature carefully). |
The same status as in the first post. I.e. I propose a configuration that allows the user to run org-ql without opening buffers for all the files that aren't already in buffers. Using a hidden work-buffer as described above. Two reasons for the patch:
For (1), the caveat is that I've only tested it on Windows, where the performance is more than 100 times faster. I suspect Linux to also be faster but maybe not by as much as 100 times.
I do agree... But setting up Emacs again and separately for this performance test was not an option. I hoped that the initial benchmark showing the difference would be enough. In the best of worlds performance tests are included in your build scripts of course, but even that will probably not capture all the relevant variations here, as I doubt Github allows automating windows builds!? Anyhow, the above is the latest status. Hope it clarifies something! |
@Whil- I meant the latest status with regard to the issues I mentioned, which would have to be resolved before this could be merged. If you don't plan to work on them yourself, then this PR probably can only serve as proof-of-concept for future work. |
Oh, didn't catch that. Sorry. You mean the interplay with The hooks issue, I have no idea how to solve that, so my time would be wasted in trying to fix it. I see it as a trade off for now. 100x performance boost for 90% of functionality, or 100% functionality and no boost. I understand if you consider this to low quality for inclusion. In such case, no hard feelings of you choose to close the pr. Personally I can still use my own fork. |
I'm not sure exactly what you mean.
I'm afraid it's a matter of correctness: this change would probably cause some searches to return incorrect results, which would be equivalent to something like 0% functionality. False negatives are the worst bug a search tool can have, and an untrustworthy tool is useless.
It's not that it's low-quality, but that it needs more work to be viable. The use-case of searching files that are not yet open in Emacs buffers is an important one, and if its performance can be improved, it should be. But due to the issues mentioned, it would have to be done carefully. Why don't you open an issue so we can discuss it in detail. (As an aside, I'd eventually like to integrate org-ql with an indexer for searching unopened files without opening them, which might obviate this issue to an extent. It's tricky, though, because of some of the things Org does automatically when opening Org files, which the indexer would have to be careful to do the equivalent of, to ensure that the same results are returned for both opened and unopened files.) |
Regarding the profiling, I think @yantar92 is right that those results don't look right. AIUI the issue is not with Assuming our intuitions are right, that should show what exactly Org is doing that takes so much time. And then, maybe some of those specific things could be optimized in Org. And from |
I also think so. Instead of intrusive More specific to the @Whil- results, there is |
What I meant was that the customization, though blunt, still has it's use and as a first step in implementing a speedup could be to always override the value of
While I'd also like to dig in to the details of this and do it better I'm not sure of what I can contribute. High risk of this becoming a real time sink, something I'm not prepared to go into right now. The "catch all" disable events solution may be to blunt to be included in Org ql, even with a disclaimer. But I'll continue to use it anyhow since there is no real option. I'll just have to stay aware of the corner cases.
I'm not even sure it's Org that is the reason. It could just as well be Emacs itself. Actually, to get a bit more evidence I ran a bunch of performance tests on only loading files into emacs. I won't go through all results but some interesting findings:
Tests: (benchmark 1 '(mapc #'find-file-noselect (directory-files "c:/PIM/Notes/" t ".org$"))) ; Sooo slow
(benchmark 1 '(mapc #'find-file-noselect (directory-files "c:/tmp/Notes/" t ".org$"))) ; 4x faster!? wtf?
(benchmark 1 '(mapc #'find-file-noselect (directory-files "c:/PIM/Notes/" t ".txt$"))) ; 2x faster than previous test
(with-temp-buffer
(org-mode)
(benchmark 1 '(mapc #'(lambda (file) (insert-file-contents file nil nil nil 'replace))
(directory-files "c:/tmp/Notes/" t ".txt$")))) ; 80x faster than previoius test To conclude: No the performance issue does not lie with org-ql. The patch lets org-ql be agnostic to the loading files into buffers performance issue. But shortcuts taken in relation to delaying mode hooks may be to blunt to allow this into the code anyhow. |
@Whil- I tried your benchmark on my Org folder with 36 large org files. I do not have file-system issues and use SSD, so I was able to progressively disable the most time-consuming org-mode functions. The benchmark time reduced from 5.5sec to 0.4sec. The benchmark without I used the following benchamrk:
The most time-consuming components are the following:
|
4f5fbc4
to
d0acc8c
Compare
059b10c
to
77b4c2b
Compare
With this change, and when the user customizes org-ql-open-buffers to it's
non-default value, org-ql will get a HUGE performance improvement on
queries when files aren't already loaded into buffers in Emacs.
This change does not modify any defaults and org-ql will work just as
before (and as slow) until org-ql-open-buffers is modified by the
user. This is due to backwards compatability.
I've tested the performance on one of my Org roam collections, with 686
files in it. Many small files. In total under 2 MB of text content. The performance
with and without this patch is as follows (on Windows 10 PC):
Test query:
The query lists 507 ID's in total.
The implementation is inspired by how Org ID caching is implemented, and the nroam implementation. I've been involved with the
design of both for full disclosure.
Haven't tested this customization very thoroughly, except with running queries on my own machines. But encourage others to try it out and report back if there are any issues!